feat!: Add working conversion webhook with cert rotation #1066

sbernauer · 2025-06-30T14:07:23Z

Description

An working example usage can be found in stackabletech/zookeeper-operator#958 (mainly look at rust/operator-binary/src/main.rs)

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

Author

Changes are OpenShift compatible
CRD changes approved
CRD documentation for all fields, following the style guide.
Integration tests passed (for non trivial changes)
Changes need to be "offline" compatible

Reviewer

Code contains useful comments
Code contains useful logging statements
(Integration-)Test cases added
Documentation added or updated. Follows the style guide.
Changelog updated
Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

Feature Tracker has been updated
Proper release label has been added

sbernauer · 2025-07-02T06:11:49Z

crates/stackable-webhook/src/tls/mod.rs

+pub const WEBHOOK_CA_LIFETIME: Duration = Duration::from_minutes_unchecked(3);
+pub const WEBHOOK_CERTIFICATE_LIFETIME: Duration = Duration::from_minutes_unchecked(2);
+pub const WEBHOOK_CERTIFICATE_ROTATION_INTERVAL: Duration = Duration::from_minutes_unchecked(1);


reminder to bump these before merging. Currently they are so low for easy testing

Techassi

Partial review, I didn't look at the CertificateResolver yet.

crates/stackable-operator/src/cli.rs

Techassi · 2025-07-03T08:06:50Z

crates/stackable-webhook/src/lib.rs

-/// A generic webhook handler receiving a request and state and sending back
-/// a response.
-///
-/// This trait is not intended to be  implemented by external crates and this
-/// library provides various ready-to-use implementations for it. One such an
-/// implementation is part of the [`ConversionWebhookServer`][1].
-///
-/// [1]: crate::servers::ConversionWebhookServer
-pub(crate) trait StatefulWebhookHandler<Req, Res, S> {
-    fn call(self, req: Req, state: S) -> Res;
-}


question: Why was this removed?

It imposed maintenance effort to me while I figured out how the code should look like.
I'm also pretty confident we are never going to need it for a conversion web-hook. Therefore I removed it to have less maintenance effort going forward.
Also, stateful conversion webhooks sounds like a potential nightmare once we want to have HA and multiple instances.
We can obviously always re-add it in the case we really need it in the future.

Alright, let's leave it as is for now. We can always re-add if needed.

This then needs a changelog entry.

Mentioned in 0903686

crates/stackable-webhook/src/servers/conversion.rs

Techassi · 2025-07-03T11:27:59Z

crates/stackable-webhook/src/servers/conversion.rs

+        options: Options,
+        client: Client,
+        field_manager: impl Into<String> + Debug,
+        operator_environment: OperatorEnvironmentOpts,


note: A good place for this might be the Options for ConversionWebhookServer.

crates/stackable-webhook/src/servers/conversion.rs

Techassi · 2025-07-03T11:46:14Z

crates/stackable-webhook/src/servers/conversion.rs

+            .context(ConvertCaToPemSnafu)?;
+
+        let crd_api: Api<CustomResourceDefinition> = Api::all(client.clone());
+        for mut crd in crds.iter().cloned() {


note: I feel like we shouldn't clone here, but instead mutate the CRDs (so that we also have an up2date version of the CRD in memory).

I also feel like we should be a little more clever on when to run all of the following code. We can skip running all of the below code for like 99% percent of the time, as the certificate is still valid and nothing needs to be adjusted in the conversion section of the CRD.

That doesn't really work out in terms of borrow checker (see below). Also I prefer the signature this way, as the only thing that we should change about the CRD is the conversion (which changes every reconcile_crds call).
Having multiple functions take owned or &mut CRDs makes this a bit less clear and some function might (accidentally) change things.
This clone is called every 7 days or so (cert lifetime), I'd say the performance is negligible here.

we should be a little more clever

Currently reconcile_crds is called for every new cert, so every 7 days or so and every time the cert changes.
Even if there would be no change, k8s would not do anything, as the object is unchanged.

diff --git a/crates/stackable-webhook/src/servers/conversion.rs b/crates/stackable-webhook/src/servers/conversion.rs index 03fc3b2..0d265b1 100644 --- a/crates/stackable-webhook/src/servers/conversion.rs +++ b/crates/stackable-webhook/src/servers/conversion.rs @@ -189,7 +189,7 @@ impl ConversionWebhookServer { Self::reconcile_crds( &client, &field_manager, - &crds, + crds.clone(), &operator_environment, &current_cert, ) @@ -240,14 +240,14 @@ impl ConversionWebhookServer { mut cert_rx: mpsc::Receiver<Certificate>, client: &Client, field_manager: &str, - crds: &[CustomResourceDefinition], + mut crds: Vec<CustomResourceDefinition>, operator_environment: &OperatorEnvironmentOptions, ) -> Result<(), ConversionWebhookError> { while let Some(current_cert) = cert_rx.recv().await { Self::reconcile_crds( client, field_manager, - crds, + &mut crds, operator_environment, &current_cert, ) @@ -261,7 +261,7 @@ impl ConversionWebhookServer { async fn reconcile_crds( client: &Client, field_manager: &str, - crds: &[CustomResourceDefinition], + crds: &mut [CustomResourceDefinition], operator_environment: &OperatorEnvironmentOptions, current_cert: &Certificate, ) -> Result<(), ConversionWebhookError> {

Co-authored-by: Techassi <[email protected]>

crates/stackable-webhook/src/tls/mod.rs

Co-authored-by: Techassi <[email protected]>

Techassi

I left a bunch of comments.

I encountered the ConversionWebhookServer which is missing the changes we talked about a few weeks back. As such, it doesn't make a whole lot of sense to continue the review before these changes are implemented. I've sent you an appropriate patch as a private message which should be a good starting point for the changes we discussed.

Techassi · 2025-08-06T14:31:10Z

crates/stackable-operator/src/cli.rs

+            "--operator-namespace",
+            "stackable-operators",
+            "--operator-service-name",
+            "foo-operator",


note: I still feel like all these CLI unit tests are pretty much useless and should be removed to speed up the testing runs.

Techassi · 2025-08-06T14:35:03Z

crates/stackable-operator/CHANGELOG.md

@@ -34,7 +34,12 @@ All notable changes to this project will be documented in this file.
 ### Changed

 - Update `kube` to `1.1.0` ([#1049]).
- BREAKING: Return type for `ListenerOperatorVolumeSourceBuilder::new()` is no onger a `Result` ([#1058]).
+- BREAKING: Return type for `ListenerOperatorVolumeSourceBuilder::new()` is no longer a `Result` ([#1058]).


note: This is already released and as such needs to be removed here.

Techassi · 2025-08-06T14:38:59Z

crates/stackable-operator/CHANGELOG.md

+- BREAKING: Require two new CLI arguments: `--operator-namespace` and `-operator-service-name`.
+  These are required, so that the operator knows what Service it needs to enter as CRD conversion webhook ([#1066]).


note: This is an addition and as such needs to be moved to the "Added" section. Also, slight reword:

Suggested change

- BREAKING: Require two new CLI arguments: `--operator-namespace` and `-operator-service-name`.

These are required, so that the operator knows what Service it needs to enter as CRD conversion webhook ([#1066]).

- BREAKING: Add two new required CLI arguments: `--operator-namespace` and `-operator-service-name`.

These two values are used to construct the service name in the CRD conversion webhook ([#1066]).

Techassi · 2025-08-06T14:42:17Z

crates/stackable-operator/CHANGELOG.md

+- BREAKING: The `ProductOperatorRun` used for CLI arguments has some field renamed for consistency ([#1066]):
+  - `telemetry_arguments` -> `telemetry`
+  - `cluster_info_opts` -> `cluster_info`


suggestion: Slight reword.

Suggested change

- BREAKING: The `ProductOperatorRun` used for CLI arguments has some field renamed for consistency ([#1066]):

- `telemetry_arguments` -> `telemetry`

- `cluster_info_opts` -> `cluster_info`

- BREAKING: Rename two fields of the `ProductOperatorRun` struct for consistency and clarity ([#1066]):

- `telemetry_arguments` -> `telemetry`

- `cluster_info_opts` -> `cluster_info`

Techassi · 2025-08-06T14:46:07Z

crates/stackable-webhook/src/servers/conversion.rs

+use kube::{
+    Api, Client, ResourceExt,
+    api::{Patch, PatchParams},
+};
+use snafu::{OptionExt, ResultExt, Snafu};
+use stackable_operator::cli::OperatorEnvironmentOptions;
+use tokio::{sync::mpsc, try_join};
 use tracing::instrument;
+use x509_cert::{
+    Certificate,
+    der::{EncodePem, pem::LineEnding},
+};


note: Please combine these imports with the others above.

Techassi · 2025-08-06T14:48:21Z

crates/stackable-webhook/src/servers/conversion.rs

+    ReconcileCRDs {
+        #[snafu(source(from(ConversionWebhookError, Box::new)))]
+        source: Box<ConversionWebhookError>,
+    },
+
+    #[snafu(display("failed to update CRD {crd_name:?}"))]
+    UpdateCRD {


note: I think we should stick with PascalCase here.

Suggested change

ReconcileCRDs {

#[snafu(source(from(ConversionWebhookError, Box::new)))]

source: Box<ConversionWebhookError>,

},

#[snafu(display("failed to update CRD {crd_name:?}"))]

UpdateCRD {

ReconcileCrd {

#[snafu(source(from(ConversionWebhookError, Box::new)))]

source: Box<ConversionWebhookError>,

},

#[snafu(display("failed to update CRD {crd_name:?}"))]

UpdateCrd {

sbernauer added 9 commits June 27, 2025 17:11

WIP

0ef29ab

Actually rotate certs

ed58563

clippy

3be083e

doctests

fd4d94b

HashMap -> Vec

b3c17b3

Set correct CA lifetime

bdbcee0

Result type

b4c22b0

fix rustdocs

31b128b

Add some docs

b84cf05

sbernauer mentioned this pull request Jun 30, 2025

tracking: CRD versioning stackabletech/issues#642

Open

88 tasks

Update rustdoc

0390680

sbernauer changed the title ~~feat(webhook): Add functioning conversion webhook with cert rotation~~ feat(webhook): Add working conversion webhook with cert rotation Jul 2, 2025

sbernauer added 3 commits July 2, 2025 08:09

Handle cert rottion loop falures

2e1d085

changelog

3f8e744

link to decision

6f0040c

sbernauer commented Jul 2, 2025

View reviewed changes

sbernauer mentioned this pull request Jul 2, 2025

feat!: Add conversion webhook stackabletech/zookeeper-operator#958

Draft

20 tasks

sbernauer self-assigned this Jul 2, 2025

fix rustdocs

941ee6b

sbernauer added this to Stackable Engineering Jul 2, 2025

sbernauer moved this to Development: In Progress in Stackable Engineering Jul 2, 2025

sbernauer moved this from Development: In Progress to Development: In Review in Stackable Engineering Jul 2, 2025

sbernauer moved this from Development: In Review to Development: Waiting for Review in Stackable Engineering Jul 2, 2025

sbernauer mentioned this pull request Jul 2, 2025

feat: Add Service on top of operators stackabletech/operator-templating#534

Open

Techassi self-requested a review July 3, 2025 06:43

Techassi moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Jul 3, 2025

sbernauer mentioned this pull request Jul 3, 2025

Use the local kubelet's cluster domain, rather than a random one #1071

Merged

8 tasks

Techassi requested changes Jul 3, 2025

View reviewed changes

Techassi changed the title ~~feat(webhook): Add working conversion webhook with cert rotation~~ feat!: Add working conversion webhook with cert rotation Jul 3, 2025

Merge remote-tracking branch 'origin/main' into feat/conversion-webhook

0c52f5f

sbernauer and others added 13 commits July 4, 2025 09:06

Rework CLI structs and names

2ec9b73

test: Remove CLI parsing from env var test

a8a2381

Move subject_alterative_dns_names into Options

9089385

changelog

4671a41

Remove mpsc in tests leftover

a3b3cc1

docs: Use result of WebhookServer::new

f8683e4

fmt

457dbf3

Update crates/stackable-webhook/src/tls/mod.rs

a39ee47

Co-authored-by: Techassi <[email protected]>

fix suggestion

6210262

capture in variable

dcb22bd

Move route into variable

f1fee5d

docs: Mention background cert rotation

caddffb

docs: Document TlsServer::new

7450d85

sbernauer requested a review from Techassi July 4, 2025 10:19

sbernauer and others added 5 commits July 4, 2025 12:23

docs: Hint on downward API env var

ac29c0e

Rewrite certificate rotation logic

51321f0

Co-authored-by: Techassi <[email protected]>

Avoid unwrap

082a59d

Make CertResolver pub, so docs pass

c032633

Add missing await

8dedf5a

Techassi reviewed Jul 8, 2025

View reviewed changes

crates/stackable-webhook/src/tls/mod.rs Outdated Show resolved Hide resolved

sbernauer and others added 3 commits July 8, 2025 15:56

mount -> project

9da5588

Update crates/stackable-webhook/src/tls/mod.rs

758202f

Co-authored-by: Techassi <[email protected]>

changelog

29b0c6c

This was referenced Jul 9, 2025

feat!(stackable-certs): Use builder pattern and support SAN entries #1044

Closed

feat!: Re-export stackable-certs and stackable-webhook crates #1074

Open

Techassi requested changes Aug 6, 2025

View reviewed changes

		- BREAKING: Require two new CLI arguments: `--operator-namespace` and `-operator-service-name`.
		These are required, so that the operator knows what Service it needs to enter as CRD conversion webhook ([#1066]).

Uh oh!

feat!: Add working conversion webhook with cert rotation #1066

Are you sure you want to change the base?

feat!: Add working conversion webhook with cert rotation #1066

Uh oh!

Conversation

sbernauer commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Definition of Done Checklist

Author

Reviewer

Acceptance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Techassi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Techassi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbernauer commented Jun 30, 2025 •

edited

Loading

Techassi left a comment •

edited

Loading